Skip to content

fix: improved external ref file loading - #3164

Open
Gavin Barron (gavinbarron) wants to merge 2 commits into
mainfrom
fix/external-ref-file-loading
Open

Gavin Barron (gavinbarron) wants to merge 2 commits into
mainfrom
fix/external-ref-file-loading

Conversation

@gavinbarron

Copy link
Copy Markdown
Collaborator

Pull Request

Description

Improves external ref file loading.

Type of Change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation update
  • Other (please describe):

Related Issue(s)

None linked.

Changes Made

  • Reuse external files during a load.
  • Bound loading depth.
  • Add regression tests and documentation.

Testing

  • Unit tests added/updated
  • Integration tests added/updated
  • Manual testing performed
  • All existing tests pass

Affected suites passed; the full solution was not run:

  • dotnet test --project test\Microsoft.OpenApi.Readers.Tests\Microsoft.OpenApi.Readers.Tests.csproj -c Release --no-restore --no-progress — passed, 665 tests.
  • dotnet test --project test\Microsoft.OpenApi.Tests\Microsoft.OpenApi.Tests.csproj -c Release --no-restore --no-progress — passed, 1,309 tests.
  • dotnet build src\Microsoft.OpenApi\Microsoft.OpenApi.csproj -c Release --no-restore — passed, both targets, no warnings.

Checklist

  • My code follows the code style of this project
  • I have performed a self-review of my own code
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes

Versions applicability

  • My change applies to the version 1.X of the library, if so PR link:
  • My change applies to the version 2.X of the library, if so PR link:
  • My change applies to the version 3.X of the library, if so PR link: this PR
  • I have evaluated the applicability of my change against the other versions above.

See the contributing guidelines for more information about how patches are applied across multiple versions.

Additional Notes

Earlier-version applicability has not been evaluated.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 357913a9-48cd-4007-897f-54956d3171aa
@gavinbarron
Gavin Barron (gavinbarron) requested a review from a team as a code owner November 6, 2026 17:00
Copilot AI balanced review requested due to automatic review settings November 6, 2026 17:00
@github-code-quality

github-code-quality Bot commented Nov 6, 2026 •

Copy link
Copy Markdown

Code coverage overview

Languages: C#

C# / code-coverage/dotnet

main a68f8be fix/external-ref-fil... f70fc29 +/-
93% 93% 0%
View coverage of the 10 most impacted files.
File main a68f8be fix/external-ref-fil... f70fc29 +/-
/home/runner/wo...piDiagnostic.cs 100% 67% -33%
/home/runner/wo...penApiWalker.cs 80% 79% -1%
/home/runner/wo...kspaceLoader.cs 100% 99% -1%
/home/runner/wo...ModelFactory.cs 88% 88% 0%
/home/runner/wo...nApiDocument.cs 91% 91% 0%
/home/runner/wo...Deserializer.cs 100% 100% 0%
/home/runner/wo...ApiWorkspace.cs 93% 94% +1%
/home/runner/wo...emaReference.cs 91% 92% +1%
/home/runner/wo...nceCollector.cs 88% 95% +7%
/home/runner/wo...StreamLoader.cs 85% 95% +10%

ℹ️ Calculations represent line coverage. Results may change as calculations are updated.


Updated November 06, 2026 21:59 UTC

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Malformed external subschema pointers can throw instead of returning diagnostics, and incomplete traversal of concrete sibling schemas leaves nested external references unresolved.

2 open issues
What changed in this PR

This PR improves external reference loading in OpenAPI.NET by reusing documents within each load, bounding traversal depth, and detecting external reference-only cycles.

Changes:

  • Adds URI-based document reuse and a configurable depth limit, defaulting to 64.
  • Updates reference resolution, schema-sibling traversal, and stream cleanup.
  • Adds regression tests and documents loading behavior.
File Description
test/​Microsoft.OpenApi.Tests/​Reader/​OpenApiReaderSettingsTests.cs Tests rejection of non-positive depth limits.
test/​Microsoft.OpenApi.Readers.Tests/​OpenApiWorkspaceTests/​OpenApiWorkspaceStreamTests.cs Tests reuse, cycles, depth limits, resolution, and stream ownership.
src/​Microsoft.OpenApi/​Services/​OpenApiWorkspace.cs Adds base-aware document lookup and exposes schema-sibling enumeration.
src/​Microsoft.OpenApi/​Reader/​Services/​OpenApiWorkspaceLoader.cs Implements document reuse, bounded traversal, and chain validation.
src/​Microsoft.OpenApi/​Reader/​Services/​OpenApiRemoteReferenceCollector.cs Collects reference holders and traverses authored schema siblings.
src/​Microsoft.OpenApi/​Reader/​Services/​DefaultStreamLoader.cs Centralizes URI resolution and fragment removal.
src/​Microsoft.OpenApi/​Reader/​OpenApiReaderSettings.cs Adds MaxExternalReferenceDepth.
src/​Microsoft.OpenApi/​Reader/​OpenApiModelFactory.cs Integrates workspace loading, source identity, and reliable stream cleanup.
src/​Microsoft.OpenApi/​PublicAPI.Unshipped.txt Records the new public depth setting.
src/​Microsoft.OpenApi/​Models/​OpenApiDocument.cs Resolves external document identities relative to the document base URI.
src/​Microsoft.OpenApi/​Models/​JsonSchemaReference.cs Identifies references without authored schema siblings.
README.md Documents reuse, depth limits, and cycle diagnostics.

Review effort: Balanced

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 357913a9-48cd-4007-897f-54956d3171aa
Copilot AI balanced review requested due to automatic review settings November 6, 2026 21:58
Comment on lines +64 to +70
foreach (var child in children)
{
if (child is not null)
{
pending.Push(child);
}
}
Comment on lines +64 to +70
foreach (var child in children)
{
if (child is not null)
{
pending.Push(child);
}
}
@sonarqubecloud

sonarqubecloud Bot commented Nov 6, 2026

Copy link
Copy Markdown

Quality Gate Failed Quality Gate failed

Failed conditions
C Reliability Rating on New Code (required ≥ A)

See analysis details on SonarQube Cloud

Catch issues before they fail your Quality Gate with our IDE extension SonarQube for IDE

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

Reference-loading defects mishandle redirected URLs, omit discriminator mappings beneath $defs or in $ref siblings, and allow local reference chains to throw uncaught indexing exceptions.

No open issues 🎉

3 previously missed issues

In code that hasn't changed since last review

Medium severity Use the final response URI as the source for relative references

src/​Microsoft.OpenApi/​Reader/​OpenApiModelFactory.cs:83

For a description that redirects to another directory or host, this records the original URL rather than the final response URI. Without a BaseUrl override, relative external references are consequently fetched from the original location, and references back to the redirected root miss the cache. Carry the final response's RequestMessage.RequestUri out of RetrieveStreamAndFormatAsync and use it as the source URI (falling back to the requested URL when unavailable).

Medium severity Load external references in discriminator mappings

src/​Microsoft.OpenApi/​Reader/​Services/​OpenApiRemoteReferenceCollector.cs:70

This traversal omits discriminator Mapping and DefaultMapping references beneath $defs or in $ref siblings, which OpenApiWalker does not traverse. With LoadExternalRefs enabled, their external documents are never fetched and the mapped targets remain unresolved. Add these authored discriminator references to the pending schemas without following Target, and add a regression test for a discriminator beneath $defs.

Medium severity Catch indexing errors when resolving local reference chains

src/​Microsoft.OpenApi/​Reader/​Services/​OpenApiWorkspaceLoader.cs:148

With LoadExternalRefs = true and an empty validation rule set, a local reference such as #/components/schemas/A/properties (where A exists) now throws IndexOutOfRangeException from LoadAsync. The new chain validation resolves local references, but this filter catches indexing exceptions only for chains containing an external reference. Catch these exceptions for local chains too, retaining the existing break behavior.

2 resolved since last review

Review effort: Balanced

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants